Skip to content

[BugFix] Scope SQL cursor continuation to original query indices under FGAC#5399

Merged
RyanL1997 merged 1 commit intoopensearch-project:mainfrom
mengweieric:fix/sql-cursor-fgac-indices
May 5, 2026
Merged

[BugFix] Scope SQL cursor continuation to original query indices under FGAC#5399
RyanL1997 merged 1 commit intoopensearch-project:mainfrom
mengweieric:fix/sql-cursor-fgac-indices

Conversation

@mengweieric
Copy link
Copy Markdown
Collaborator

Summary

  • Legacy (V1) SQL cursor continuation built its SearchRequest with new SearchRequest() (no indices). Under OpenSearch Security Fine-Grained Access Control, an indices-less SearchRequest resolves to a wildcard during authorization, so users who only have permissions on the queried index get 403 no permissions for [indices:data/read/search] on page 2 and later. Page 1 worked because the initial SearchRequest was built via SearchRequestBuilder and carried the concrete indices.
  • Persist the resolved indices into DefaultCursor at page-1 time and scope the continuation SearchRequest with them. Security now authorizes against the same concrete indices across every page.

Root cause

  1. PrettyFormatRestExecutor.buildProtocolWithPagination creates the page-1 SearchRequest via queryAction.getRequestBuilder(), which carries queryAction.getSelect().getIndexArr(). Security sees concrete indices and grants.
  2. On continuation, CursorResultExecutor.handleDefaultCursorRequest built new SearchRequest() with no indices and only a SearchSourceBuilder + PIT id. Security resolves the empty indices array to the cluster-wide wildcard, evaluates the user's role against *, and denies.

The PIT is correctly scoped on both page 1 and continuation; the denial happens in Security's pre-PIT authorization of the outer SearchRequest.

Fix

  • DefaultCursor: new String[] indices field, serialized under a new short cursor-payload key "x". Deserialization uses optJSONArray and defaults to new String[0] when the key is absent, so cursor tokens issued by pre-fix nodes continue to deserialize cleanly after a rolling upgrade.
  • PrettyFormatRestExecutor.createCursorWithPit: populates cursor.setIndices(queryAction.getSelect().getIndexArr()) at page-1 time.
  • CursorResultExecutor.handleDefaultCursorRequest: continuation SearchRequest now new SearchRequest(cursor.getIndices()).

Scope: only the legacy V1 cursor path changes. The V2 cursor path (buildProtocolWithPagination going through SearchRequestBuilder.get()) already carried indices end-to-end and is untouched; PaginationIT stays green.

Back-compat

  • Old in-flight cursor tokens (no "x" field) deserialize with indices=[]. Those continuations still hit the original bug for FGAC users but behave exactly as they did before the fix, i.e. nothing gets worse during a rolling upgrade.
  • New cursor tokens written by upgraded nodes contain "x" and authorize correctly on every page.
  • Non-FGAC clusters are unaffected in either direction.

Test plan

  • ./gradlew :legacy:spotlessCheck :integ-test:spotlessCheck
  • ./gradlew :legacy:test (all pass, including 4 new DefaultCursorTest cases: serialization of indices, null-indices default, round-trip, legacy-cursor-without-x back-compat)
  • ./gradlew build -x integTest -x integTestWithSecurity -x yamlRestTest (all module unit tests)
  • ./gradlew :integ-test:integTestWithSecurity --tests org.opensearch.sql.security.SQLCursorPermissionsIT (both tests pass)
  • Verified the new IT catches the bug: with the three source edits stashed, cursorPaginationUnderFgacSucceedsAcrossPages fails with the exact customer symptom (403 no permissions for [indices:data/read/search]), then passes once the fix is restored.
  • ./gradlew :integ-test:integTest --tests org.opensearch.sql.legacy.CursorIT (54/54 pass; serialization change is back-compat with existing V1 cursor flows)
  • ./gradlew :integ-test:integTest --tests org.opensearch.sql.sql.PaginationIT (24/24 pass; V2 cursor path unaffected)

Files changed

  • legacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.java
  • legacy/src/main/java/org/opensearch/sql/legacy/executor/cursor/CursorResultExecutor.java
  • legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java
  • legacy/src/test/java/org/opensearch/sql/legacy/unittest/cursor/DefaultCursorTest.java
  • integ-test/src/test/java/org/opensearch/sql/security/SQLCursorPermissionsIT.java (new)

…r FGAC

The legacy (V1) SQL cursor continuation path built its SearchRequest with
`new SearchRequest()` (no indices). Under OpenSearch Security Fine-Grained
Access Control, an indices-less SearchRequest resolves to a wildcard during
authorization, so users who only have permissions on the queried index are
denied `indices:data/read/search` on the second and subsequent pages. Page 1
worked because the initial SearchRequest was built via SearchRequestBuilder
and carried the concrete indices.

Persist the resolved indices into DefaultCursor at page-1 time and pass them
to the continuation SearchRequest so Security authorizes against the same
concrete indices across all pages. The cursor payload uses a new short key
'x' and falls back to an empty array when absent, so cursor tokens issued by
pre-fix nodes continue to deserialize cleanly.

Regression coverage:
- Unit test for DefaultCursor round-trip including the legacy-without-x case.
- Integration test SQLCursorPermissionsIT that exercises V1 cursor paging
  under an FGAC role scoped to a single index. The new test fails on HEAD
  with 403 on page 2 and passes after the fix.

Signed-off-by: Eric Wei <[email protected]>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 1, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Core FGAC cursor fix with unit tests

Relevant files:

  • legacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.java
  • legacy/src/main/java/org/opensearch/sql/legacy/executor/cursor/CursorResultExecutor.java
  • legacy/src/main/java/org/opensearch/sql/legacy/executor/format/PrettyFormatRestExecutor.java
  • legacy/src/test/java/org/opensearch/sql/legacy/unittest/cursor/DefaultCursorTest.java

Sub-PR theme: Integration test for SQL cursor FGAC regression

Relevant files:

  • integ-test/src/test/java/org/opensearch/sql/security/SQLCursorPermissionsIT.java

⚡ Recommended focus areas for review

Test Reliability

The initialized flag is an instance field, but SQLIntegTestCase may create a new instance per test method. If so, createSecurityRolesAndUsers() will be called multiple times, and the guard if (initialized) return; will never fire. This could cause redundant role/user creation requests or failures if the security API returns an error on duplicate creation (rather than 200/201). Consider using a @BeforeClass static flag or assumeTrue to guard idempotent setup.

private boolean initialized = false;

@Override
protected void init() throws Exception {
  loadIndex(Index.ACCOUNT);
  createSecurityRolesAndUsers();
}

private void createSecurityRolesAndUsers() throws IOException {
  if (initialized) {
    return;
  }
  createRole(ACCOUNT_ROLE, TEST_INDEX_ACCOUNT);
  createUser(ACCOUNT_USER, ACCOUNT_ROLE);
  initialized = true;
}
Hardcoded Row Count

The test asserts exactly 5 pages based on 234 rows / 50 per page. This assumes the TEST_INDEX_ACCOUNT index always contains exactly 1000 rows (or at least 234). If the index has fewer rows, the cursor may exhaust earlier and the assertion will fail. The test should either assert pages >= 2 (to confirm at least one continuation page succeeded) or document the exact expected row count more explicitly.

  // 234 rows / 50 per page = 5 pages
  assertEquals("expected 5 V1 cursor pages under FGAC", 5, pages);
}
Charset Assumption

In the legacy-cursor backward-compatibility test, json.toString().getBytes() and new String(Base64.getDecoder().decode(payload)) use the platform default charset. If the platform charset is not UTF-8, encoding/decoding may be inconsistent. Explicit StandardCharsets.UTF_8 should be used. The same concern applies to decodePayload in the test.

}

public static DefaultCursor from(String cursorId) {
Empty Indices Fallback

When cursor.getIndices() returns an empty array (e.g., for cursors issued by pre-fix nodes after a rolling upgrade), the SearchRequest is constructed with new SearchRequest(new String[0]). Depending on the OpenSearch version, an empty indices array may still resolve to a wildcard, meaning the FGAC issue could persist for in-flight cursors from pre-fix nodes. This edge case should be documented or handled explicitly.

String[] indices = cursor.getIndices();
SearchRequest searchRequest = new SearchRequest(indices == null ? new String[0] : indices);

@mengweieric mengweieric added bug Something isn't working SQL skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. skip-diff-reviewer Maintainer to skip code-diff-reviewer check, after reviewing issues in AI analysis. labels May 1, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 1, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fallback to index pattern for legacy cursors

When indices is an empty array (e.g., deserialized from a legacy cursor without the
"x" field), the SearchRequest will still be constructed with no indices, which
resolves to a wildcard under FGAC and causes the same 403 denial the fix intends to
prevent. Consider falling back to cursor.getIndexPattern() when the indices array is
empty, so legacy cursors still work correctly after upgrade.

legacy/src/main/java/org/opensearch/sql/legacy/executor/cursor/CursorResultExecutor.java [120-121]

 String[] indices = cursor.getIndices();
-SearchRequest searchRequest = new SearchRequest(indices == null ? new String[0] : indices);
+SearchRequest searchRequest;
+if (indices != null && indices.length > 0) {
+  searchRequest = new SearchRequest(indices);
+} else {
+  // Fallback for legacy cursors that predate the indices field
+  searchRequest = new SearchRequest(cursor.getIndexPattern().split("\\|"));
+}
Suggestion importance[1-10]: 7

__

Why: This is a valid concern: when indices is empty (legacy cursor without "x" field), the fix still creates a SearchRequest with no indices, which would still fail under FGAC. Falling back to indexPattern would handle the upgrade scenario correctly. However, the test deserializeLegacyCursorWithoutIndicesDefaultsToEmptyArray explicitly tests that empty array is returned for legacy cursors, suggesting the author may have intended a different fallback strategy.

Medium
Safely encode cursor value in JSON body

The cursor string may contain characters that are not safe to embed directly in a
JSON string literal (e.g., +, /, = from Base64, or special characters). Using
String.format to inject the cursor value without escaping can produce malformed JSON
and cause unexpected failures. Use a JSONObject to build the request body so the
cursor value is properly escaped.

integ-test/src/test/java/org/opensearch/sql/security/SQLCursorPermissionsIT.java [181-190]

 int pages = 1;
 while (!cursor.isEmpty()) {
-  JSONObject next =
-      executeSqlAsUser(
-          String.format(Locale.ROOT, "{\"cursor\": \"%s\"}", cursor), ACCOUNT_USER);
+  JSONObject body = new JSONObject();
+  body.put("cursor", cursor);
+  JSONObject next = executeSqlAsUser(body.toString(), ACCOUNT_USER);
   cursor = next.optString("cursor", "");
   pages++;
 }
 // 234 rows / 50 per page = 5 pages
 assertEquals("expected 5 V1 cursor pages under FGAC", 5, pages);
Suggestion importance[1-10]: 6

__

Why: The cursor value (Base64-encoded) can contain characters like +, /, and = that are safe in JSON strings but the suggestion is valid for robustness. Using JSONObject to build the request body ensures proper escaping and avoids potential malformed JSON issues.

Low
General
Use explicit charset for Base64 encode/decode

json.toString().getBytes() uses the platform's default charset, which may differ
across environments and cause decoding failures. Explicitly specify
StandardCharsets.UTF_8 for both encoding and decoding to ensure consistent behavior.

legacy/src/test/java/org/opensearch/sql/legacy/unittest/cursor/DefaultCursorTest.java [170-172]

-JSONObject json = new JSONObject(new String(Base64.getDecoder().decode(payload)));
+JSONObject json = new JSONObject(new String(Base64.getDecoder().decode(payload), java.nio.charset.StandardCharsets.UTF_8));
 json.remove("x");
-String legacyPayload = Base64.getEncoder().encodeToString(json.toString().getBytes());
+String legacyPayload = Base64.getEncoder().encodeToString(json.toString().getBytes(java.nio.charset.StandardCharsets.UTF_8));
Suggestion importance[1-10]: 4

__

Why: Using explicit StandardCharsets.UTF_8 is a good practice to avoid platform-dependent behavior, but this is a test file and the risk of charset mismatch causing failures in practice is low. It's a minor improvement to code robustness.

Low

@RyanL1997 RyanL1997 merged commit c0ac784 into opensearch-project:main May 5, 2026
43 of 48 checks passed
ahkcs added a commit to ahkcs/sql that referenced this pull request May 8, 2026
Bring the analytics-engine catch-up PR up to current upstream/main by
resolving conflicts introduced by 4 main commits since 2026-04-30:

- opensearch-project#5419 (LENGTH/REGEXP_REPLACE/DATE_TRUNC unified function spec)
- opensearch-project#5408 (datetime type normalization)
- opensearch-project#5414 (Gradle wrapper bump + @ignore exclusion)
- opensearch-project#5399 (FGAC-scoped SQL cursor continuation)

Resolutions:

api/spec/* (LanguageSpec, UnifiedFunctionSpec, UnifiedPplSpec,
UnifiedSqlSpec): took main. Main is a strict superset — adds
postAnalysisRules and preCompilationRules extension points, the new
FunctionSpecBuilder DSL, SCALAR category for length/regexp_replace/
date_trunc, the DatetimeExtension on PPL spec, and the CoreExtension
wiring on SQL spec. PR's RELEVANCE category is preserved unchanged.

api/UnifiedQueryPlanner.java, api/compiler/UnifiedQueryCompiler.java:
took main. Both adopt the new postAnalysisRules / preCompilationRules
hooks introduced in opensearch-project#5408 / opensearch-project#5419.

core/executor/QueryService.java: composed both sides — kept HEAD's
CalciteClassLoaderHelper.withCalciteClassLoader wrapper around main's
StageErrorHandler stage tracking. Same pattern as the original PR
resolution; both improvements are orthogonal.

plugin/SQLPlugin.java: kept HEAD imports for ExplainResponse and
QueryType (referenced by createSqlAnalyticsRouter, which only exists
on the feature branch).

plugin/transport/TransportPPLQueryAction.java: kept HEAD's
queryPlanExecutor parameter alongside main's extensionsHolder
parameter, since both are referenced in the constructor body.

legacy/plugin/RestSqlAction.java: took HEAD. The 3-way merge produced
a duplicated handleException/getRawErrorCode block; HEAD already
contained both the delegateToV2Engine refactor and the ErrorReport
unwrap from main, so HEAD is the correct superset.

Build: :api, :core, :opensearch-sql-plugin, :legacy compileJava and
unit tests pass; spotlessCheck passes.

Signed-off-by: Kai Huang <[email protected]>
ahkcs added a commit to ahkcs/sql that referenced this pull request May 8, 2026
…ation

Brings the catch-up branch up to current upstream/main (4 commits since
this PR was opened) and current feature/mustang-ppl-integration (9
commits since this PR was opened), so the PR is mergeable into
feature/mustang-ppl-integration without conflicts.

Squashed (rather than two real merge commits) for the same DCO reason
the original commit was squashed: upstream commits authored by many
contributors with inconsistent or missing Signed-off-by trailers would
otherwise be brought into this PR's history.

Newer main commits absorbed (4):
- opensearch-project#5419 (LENGTH/REGEXP_REPLACE/DATE_TRUNC unified function spec)
- opensearch-project#5408 (datetime type normalization)
- opensearch-project#5414 (Gradle wrapper bump + @ignore exclusion)
- opensearch-project#5399 (FGAC-scoped SQL cursor continuation)

Newer feature commits absorbed (9):
- opensearch-project#5403 (analytics-engine optional dependency — major rewiring)
- opensearch-project#5407 (Carry CalciteEvalCommandIT through helper-managed index path)
- opensearch-project#5413 (Default plugins.calcite.enabled=true on unified path)
- opensearch-project#5415, opensearch-project#5416, opensearch-project#5417, opensearch-project#5409, opensearch-project#5400, opensearch-project#5406 (smaller carryovers + bumps)

Conflict resolutions (10 from main side, 3 from feature side):

api/spec/* (LanguageSpec, UnifiedFunctionSpec, UnifiedPplSpec,
UnifiedSqlSpec): took main. Main is a strict superset — adds
postAnalysisRules and preCompilationRules extension points, the new
FunctionSpecBuilder DSL, SCALAR category for length/regexp_replace/
date_trunc, the DatetimeExtension on PPL spec, and the CoreExtension
wiring on SQL spec. PR's RELEVANCE category is preserved unchanged.

api/UnifiedQueryPlanner.java, api/compiler/UnifiedQueryCompiler.java:
took main. Both adopt the new postAnalysisRules / preCompilationRules
hooks introduced in opensearch-project#5408 / opensearch-project#5419.

core/executor/QueryService.java: composed both sides — kept HEAD's
CalciteClassLoaderHelper.withCalciteClassLoader wrapper around main's
StageErrorHandler stage tracking. Same pattern as the original PR
resolution; both improvements are orthogonal.

legacy/plugin/RestSqlAction.java: took HEAD. The 3-way merge produced
a duplicated handleException/getRawErrorCode block; HEAD already
contained both the delegateToV2Engine refactor and the ErrorReport
unwrap from main, so HEAD is the correct superset.

integ-test/build.gradle: took feature. Both sides added the same
@ignore exclusion block; feature has alphabetical ordering and a more
detailed comment explaining the Gradle 9.4.1 TestEventReporterAsListener
cast bug.

integ-test/.../CalciteEvalCommandIT.java: composed both sides. Took
feature's helper-managed test_eval provisioning (createIndexByRestClient
+ isIndexExist guard, from opensearch-project#5407) so analytics-engine compatibility runs
get a parquet-backed index. Added back PR HEAD's test_eval_agent setup
(needed by the dotted-path eval tests for opensearch-project#5351) wrapped in its own
isIndexExist guard for the same parquet-aware idempotency.

plugin/.../TransportPPLQueryAction.java: took feature. PR opensearch-project#5403 made
analytics-engine an optional dependency by moving QueryPlanExecutor
from a required constructor parameter to an @Inject(optional=true)
setter. Feature's design supersedes our prior wiring.

plugin/.../SQLPlugin.java: took feature. The same opensearch-project#5403 simplification
removed loadExtensions/EngineExtensionsHolder/executionEngineExtensions
plumbing (no longer needed once analytics-engine is optionally bound).
Feature retains the createSqlAnalyticsRouter method this PR introduced.

plugin/.../config/EngineExtensionsHolder.java: deleted. Unreferenced
after taking feature's SQLPlugin/TransportPPLQueryAction; not present
on feature branch.

Build: :api, :core, :opensearch-sql-plugin, :legacy compileJava +
:integ-test compileTestJava all pass; unit tests pass; spotlessCheck
clean.

Signed-off-by: Kai Huang <[email protected]>
ahkcs added a commit to ahkcs/sql that referenced this pull request May 8, 2026
Single squashed commit on top of feature/mustang-ppl-integration that
absorbs upstream/main's commits not yet on the feature branch. Replaces
the prior catch-up squash (opensearch-project#5396 base + the original af831d3 rebase
commit) so this PR is a fast-forward into feature/mustang-ppl-integration.

Squashed (rather than a merge commit) because upstream main commits
were authored by many contributors with inconsistent or missing
Signed-off-by trailers; DCO would otherwise reject those commits.

Main commits absorbed (54 since divergence; 4 since the original
catch-up squash was made on 2026-04-30):
- opensearch-project#5419 (LENGTH/REGEXP_REPLACE/DATE_TRUNC unified function spec)
- opensearch-project#5408 (datetime type normalization)
- opensearch-project#5414 (Gradle wrapper bump + @ignore exclusion)
- opensearch-project#5399 (FGAC-scoped SQL cursor continuation)
- opensearch-project#5394 (SQL Vector Search), opensearch-project#5361 (OpenSearch 3.7), opensearch-project#5360 (unified SQL
  language spec), opensearch-project#5240 (PPL Union), and 46 others.

Conflict resolutions:

api/spec/* (LanguageSpec, UnifiedFunctionSpec, UnifiedPplSpec,
UnifiedSqlSpec): took main. Main is a strict superset — adds
postAnalysisRules and preCompilationRules extension points, the new
FunctionSpecBuilder DSL, SCALAR category for length/regexp_replace/
date_trunc, the DatetimeExtension on PPL spec, and the CoreExtension
wiring on SQL spec. RELEVANCE category is preserved unchanged.

api/UnifiedQueryPlanner.java, api/compiler/UnifiedQueryCompiler.java:
took main. Both adopt the new postAnalysisRules / preCompilationRules
hooks introduced in opensearch-project#5408 / opensearch-project#5419.

core/executor/QueryService.java: composed both sides — kept feature's
CalciteClassLoaderHelper.withCalciteClassLoader wrapper around main's
StageErrorHandler stage tracking; both improvements are orthogonal.

legacy/plugin/RestSqlAction.java: took feature. The 3-way merge
produced a duplicated handleException/getRawErrorCode block; feature
already contained both the delegateToV2Engine refactor and the
ErrorReport unwrap from main, so feature is the correct superset.

CLAUDE.md, docs/user/ppl/functions/condition.md: took main.

explain_streamstats_global{,_null_bucket}.yaml: took main (post-opensearch-project#5359
shape).

core/CalciteRelNodeVisitor + utils/PlanUtils: took main (collation
utility hoisted from CalciteRelNodeVisitor.backtrackForCollation into
PlanUtils.findInputCollation).

integ-test/CalciteNoPushdownIT.java: added CalciteMixedFieldTypeIT.

ppl/antlr/OpenSearchPPLParser.g4: added unionCommand.

ppl/calcite/CalcitePPLStreamstatsTest.java: added
testMultipleStreamstatsWithWindow.

integ-test/build.gradle: took feature. Both sides added the same
@ignore exclusion block; feature has alphabetical ordering and a more
detailed comment explaining the Gradle 9.4.1 cast bug.

integ-test/.../CalciteEvalCommandIT.java: composed both sides. Took
feature's helper-managed test_eval provisioning (createIndexByRestClient
+ isIndexExist guard, from opensearch-project#5407) so analytics-engine compatibility runs
get a parquet-backed index. Added the test_eval_agent setup (needed by
the dotted-path eval tests for opensearch-project#5351) wrapped in its own isIndexExist
guard for the same parquet-aware idempotency.

plugin/.../TransportPPLQueryAction.java, plugin/.../SQLPlugin.java:
took feature. PR opensearch-project#5403 made analytics-engine an optional dependency by
moving QueryPlanExecutor from a required constructor parameter to an
@Inject(optional=true) setter, and removed the loadExtensions /
EngineExtensionsHolder / executionEngineExtensions plumbing. Feature
retains the createSqlAnalyticsRouter method this catch-up introduced.

plugin/.../config/EngineExtensionsHolder.java: deleted (unreferenced
post-opensearch-project#5403; not present on feature).

Build: :api, :core, :opensearch-sql-plugin, :legacy compileJava +
:integ-test compileTestJava all pass; unit tests pass; spotlessCheck
clean.

Signed-off-by: Kai Huang <[email protected]>
ahkcs added a commit that referenced this pull request May 8, 2026
…5397)

Single squashed commit on top of feature/mustang-ppl-integration that
absorbs upstream/main's commits not yet on the feature branch. Replaces
the prior catch-up squash (#5396 base + the original af831d3 rebase
commit) so this PR is a fast-forward into feature/mustang-ppl-integration.

Squashed (rather than a merge commit) because upstream main commits
were authored by many contributors with inconsistent or missing
Signed-off-by trailers; DCO would otherwise reject those commits.

Main commits absorbed (54 since divergence; 4 since the original
catch-up squash was made on 2026-04-30):
- #5419 (LENGTH/REGEXP_REPLACE/DATE_TRUNC unified function spec)
- #5408 (datetime type normalization)
- #5414 (Gradle wrapper bump + @ignore exclusion)
- #5399 (FGAC-scoped SQL cursor continuation)
- #5394 (SQL Vector Search), #5361 (OpenSearch 3.7), #5360 (unified SQL
  language spec), #5240 (PPL Union), and 46 others.

Conflict resolutions:

api/spec/* (LanguageSpec, UnifiedFunctionSpec, UnifiedPplSpec,
UnifiedSqlSpec): took main. Main is a strict superset — adds
postAnalysisRules and preCompilationRules extension points, the new
FunctionSpecBuilder DSL, SCALAR category for length/regexp_replace/
date_trunc, the DatetimeExtension on PPL spec, and the CoreExtension
wiring on SQL spec. RELEVANCE category is preserved unchanged.

api/UnifiedQueryPlanner.java, api/compiler/UnifiedQueryCompiler.java:
took main. Both adopt the new postAnalysisRules / preCompilationRules
hooks introduced in #5408 / #5419.

core/executor/QueryService.java: composed both sides — kept feature's
CalciteClassLoaderHelper.withCalciteClassLoader wrapper around main's
StageErrorHandler stage tracking; both improvements are orthogonal.

legacy/plugin/RestSqlAction.java: took feature. The 3-way merge
produced a duplicated handleException/getRawErrorCode block; feature
already contained both the delegateToV2Engine refactor and the
ErrorReport unwrap from main, so feature is the correct superset.

CLAUDE.md, docs/user/ppl/functions/condition.md: took main.

explain_streamstats_global{,_null_bucket}.yaml: took main (post-#5359
shape).

core/CalciteRelNodeVisitor + utils/PlanUtils: took main (collation
utility hoisted from CalciteRelNodeVisitor.backtrackForCollation into
PlanUtils.findInputCollation).

integ-test/CalciteNoPushdownIT.java: added CalciteMixedFieldTypeIT.

ppl/antlr/OpenSearchPPLParser.g4: added unionCommand.

ppl/calcite/CalcitePPLStreamstatsTest.java: added
testMultipleStreamstatsWithWindow.

integ-test/build.gradle: took feature. Both sides added the same
@ignore exclusion block; feature has alphabetical ordering and a more
detailed comment explaining the Gradle 9.4.1 cast bug.

integ-test/.../CalciteEvalCommandIT.java: composed both sides. Took
feature's helper-managed test_eval provisioning (createIndexByRestClient
+ isIndexExist guard, from #5407) so analytics-engine compatibility runs
get a parquet-backed index. Added the test_eval_agent setup (needed by
the dotted-path eval tests for #5351) wrapped in its own isIndexExist
guard for the same parquet-aware idempotency.

plugin/.../TransportPPLQueryAction.java, plugin/.../SQLPlugin.java:
took feature. PR #5403 made analytics-engine an optional dependency by
moving QueryPlanExecutor from a required constructor parameter to an
@Inject(optional=true) setter, and removed the loadExtensions /
EngineExtensionsHolder / executionEngineExtensions plumbing. Feature
retains the createSqlAnalyticsRouter method this catch-up introduced.

plugin/.../config/EngineExtensionsHolder.java: deleted (unreferenced
post-#5403; not present on feature).

Build: :api, :core, :opensearch-sql-plugin, :legacy compileJava +
:integ-test compileTestJava all pass; unit tests pass; spotlessCheck
clean.

Signed-off-by: Kai Huang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. skip-diff-reviewer Maintainer to skip code-diff-reviewer check, after reviewing issues in AI analysis. SQL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants